feat: deletion APIs + secure_delete for disappearing messages (3/3) by dannym-arx · Pull Request #315 · marmot-protocol/mdk

dannym-arx · 2026-05-22T11:47:37Z

Final piece of the 3-PR split for disappearing messages. The first PR wired the field through the MLS extension, the second added validation and the NIP-40 auto-expiration tag, and this one hands clients the deletion plumbing plus a SQLite hardening that keeps deleted rows from lingering on disk.

This is the part where marmots learn to clean up after themselves.

What's in it

New MessageStorage trait methods:

delete_message(group_id, event_id) -> bool: single-message delete
delete_messages_before_timestamp(group_id, before) -> usize: bulk delete by age (caller computes now - duration)
delete_processed_messages_for_group(group_id) -> usize: clears the dedup metadata for a group

Implementations across mdk-memory-storage and mdk-sqlite-storage. The memory backend scopes messages_cache eviction to the owning group, so a coincident EventId in another group can't get yanked out by accident.

MDK public API:
Matching wrapper methods on MDK so clients can call these without going through the storage trait directly.

UniFFI:
All three deletion methods surfaced over the FFI boundary for Kotlin and Swift consumers.

SQLite hardening:
PRAGMA secure_delete = ON on every connection init. When a row is deleted, SQLite overwrites the freed bytes with zeros instead of just unlinking them. An expired disappearing message no longer sits recoverable in the database file for a forensic tool to lift later.

Part of the split

Part 1: feat: extension v3 wire format + disappearing_message_secs schema (1/3) #258, MLS extension v3 wire format
Part 2: feat: validation + NIP-40 expiration + UniFFI surface for disappearing messages (2/3) #306, validation, NIP-40 auto-expiration, tri-state update
Part 3: this PR, deletion APIs and secure_delete

Testing

Tests added for each new trait method on both storage backends. PRAGMA secure_delete verification test. just precommit green across all features.

What this doesn't do

MDK doesn't run a background sweeper. Clients call delete_messages_before_timestamp on whatever cadence makes sense for them. The primitives ship here; clients pick the policy.

⚠️ Security-sensitive changes

This PR completes the disappearing messages feature by adding per-message and bulk deletion APIs across storage layers and exposing them through MDK and UniFFI, and it hardens SQLite by enabling PRAGMA secure_delete to reduce forensic recoverability of deleted rows. Clients are expected to call the new bulk-deletion API on their chosen cadence because MDK does not run a background sweeper.

What changed:

Added three new MessageStorage trait methods: delete_message(group_id, event_id) -> Result<bool, MessageError>, delete_messages_before_timestamp(group_id, before: Timestamp) -> Result<usize, MessageError>, and delete_processed_messages_for_group(group_id) -> Result<usize, MessageError> (crate: mdk-storage-traits).
Implemented the three deletion methods in mdk-memory-storage with messages_cache eviction scoped to the owning group to avoid evicting another group's coincident EventId.
Implemented the three deletion methods in mdk-sqlite-storage including corresponding SQL deletions and tests.
Exposed matching wrapper methods on MDK: delete_message, delete_messages_before_timestamp, and delete_processed_messages_for_group (crate: mdk-core).
Added UniFFI bindings for the three deletion APIs so Kotlin/Swift consumers can call delete_message, delete_messages_before_timestamp (u64 seconds → returns u32), and delete_processed_messages_for_group (crate: mdk-uniffi).
Updated changelogs across mdk-core, mdk-memory-storage, mdk-sqlite-storage, mdk-storage-traits, and mdk-uniffi to document the additions.

Security impact:

Enabled SQLite PRAGMA secure_delete = ON on every connection initialization (including in-memory constructors) so deleted rows are overwritten with zeros to reduce forensic recovery of expired disappearing messages.
No changes to cryptographic algorithms, key derivation, key handling, or identity validation are introduced by this PR.

API surface:

Breaking change: all MessageStorage implementors must implement the three new deletion methods (trait update in mdk-storage-traits).
New public MDK methods: delete_message(&self, group_id: &GroupId, event_id: &EventId) -> Result<bool, Error>, delete_messages_before_timestamp(&self, group_id: &GroupId, before: Timestamp) -> Result<usize, Error>, and delete_processed_messages_for_group(&self, group_id: &GroupId) -> Result<usize, Error> (crate: mdk-core).
New UniFFI exports: delete_message(mls_group_id: String, event_id: String) -> Result<bool, MdkUniffiError>, delete_messages_before_timestamp(mls_group_id: String, before_secs: u64) -> Result<u32, MdkUniffiError>, and delete_processed_messages_for_group(mls_group_id: String) -> Result<u32, MdkUniffiError> (crate: mdk-uniffi).
No storage schema or MLS protocol wire-format changes are introduced in this PR.

Testing:

Added unit tests for each new deletion method in both mdk-memory-storage and mdk-sqlite-storage covering deletion counts, idempotent behavior when deleting non-existent messages, timestamp-based deletions, and group scoping.
Added a test verifying PRAGMA secure_delete is enabled for SQLite connections.
All new tests pass under just precommit.

Adds local deletion primitives so clients can enforce NIP-40 expiration on their own schedule, and hardens SQLite against forensic recovery of deleted content. New MessageStorage trait methods: - delete_message(group_id, event_id) -> bool - delete_messages_before_timestamp(group_id, before) -> usize - delete_processed_messages_for_group(group_id) -> usize Implementations in mdk-memory-storage and mdk-sqlite-storage. The memory backend scopes messages_cache eviction to the owning group to avoid evicting another group's entry under coincident EventIds. mdk-core exposes matching public methods on MDK. UniFFI surfaces all three over the FFI boundary. SQLite connection init now sets PRAGMA secure_delete = ON so deleted rows (including expired disappearing messages) are overwritten with zeros on disk. Final part of the 3-PR split: - Part 1: #258 (MLS extension v3 wire format) - Part 2: #306 (validation + NIP-40 auto-expiration + tri-state)

coderabbitai · 2026-05-22T11:47:49Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 03513b67-8eef-4467-8479-c6ab8e6af3b7

📥 Commits

Reviewing files that changed from the base of the PR and between 2185662 and 9e79651.

📒 Files selected for processing (5)

crates/mdk-core/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-uniffi/CHANGELOG.md

✅ Files skipped from review due to trivial changes (2)

crates/mdk-core/CHANGELOG.md
crates/mdk-uniffi/CHANGELOG.md

📝 Walkthrough

Walkthrough

This PR adds three deletion APIs—delete_message (single event), delete_messages_before_timestamp (bulk by creation time), and delete_processed_messages_for_group (processed-message metadata)—across the MessageStorage trait, MDK core, memory and SQLite storage implementations, and UniFFI bindings; SQLite now enables PRAGMA secure_delete on init.

Changes

Granular Message Deletion Feature

Layer / File(s)	Summary
Storage trait contract and interface documentation `crates/mdk-storage-traits/src/messages/mod.rs`, `crates/mdk-storage-traits/CHANGELOG.md`	`MessageStorage` trait gains three deletion methods: per-message by event ID, bulk by timestamp, and processed-message cleanup; `delete_messages_for_group` docs clarified.
Core MDK wrapper API `crates/mdk-core/src/groups.rs`, `crates/mdk-core/CHANGELOG.md`	`MDK<Storage>` adds public methods delegating deletion to storage and mapping errors to `Error::Group`.
Memory storage implementation with tests `crates/mdk-memory-storage/src/messages.rs`, `crates/mdk-memory-storage/CHANGELOG.md`	`MdkMemoryStorage` implements group-scoped single and bulk deletions and processed-message cleanup with tests for single deletion, timestamp cutoff behavior, processed-message isolation, and regression guarding.
SQLite storage implementation, secure delete, and tests `crates/mdk-sqlite-storage/src/messages.rs`, `crates/mdk-sqlite-storage/src/lib.rs`, `crates/mdk-sqlite-storage/CHANGELOG.md`	SQLite storage implements deletions with SQL DELETE queries scoped by group and timestamp; enables `PRAGMA secure_delete = ON` for file and in-memory DBs; adds tests for deletion semantics and secure-delete enablement.
UniFFI mobile client bindings `crates/mdk-uniffi/src/lib.rs`, `crates/mdk-uniffi/CHANGELOG.md`	Exposes UniFFI APIs for single-message deletion, timestamp-based bulk deletion, and processed-message cleanup with input parsing and error mapping for mobile bindings.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

feat: add delete_messages_for_group and delete_group public methods for local chat/group cleanup #250: Introduced foundational delete_messages_for_group and delete_group local-deletion APIs; this PR extends those with per-message and timestamp-based deletion methods.

Suggested labels

security, breaking-change, storage

Suggested reviewers

mubarakcoded
jgmontoya

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding deletion APIs and secure_delete SQLite pragma for disappearing messages, with the part number indicating this is the final piece of a multi-part feature.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No Sensitive Identifier Leakage	✅ Passed	No sensitive identifiers found in tracing macros, format strings, panic messages, or Debug implementations in the new deletion method implementations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch disappearing-messages-part-3

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

stage-review · 2026-05-22T11:48:04Z

Ready to review this PR? Stage has broken it down into 5 individual chapters for you:

	Title
1	Define deletion methods in MessageStorage trait
2	Implement deletion in memory storage backend
3	Implement deletion and secure_delete in SQLite
4	Expose deletion APIs through MDK core
5	Surface deletion APIs to UniFFI bindings

_{Chapters generated by Stage for commit 9e79651 on May 22, 2026 11:52am UTC.}

github-actions · 2026-05-22T11:49:58Z

❌ Coverage: 94.51% → 94.44% (-0.07%)

…PR refs The cherry-pick from the original combined PR #253 placed the deletion-API and secure_delete entries in the [0.8.0] section because that PR predated the 0.8.0 release cut. Move them to Unreleased and append the #315 PR link, matching the convention used for Part 1 (#258) and Part 2 (#306). Also adds the missing mdk-uniffi changelog entry for the new deletion bindings.

coderabbitai

Actionable comments posted: 5

🧹 Nitpick comments (3)

crates/mdk-memory-storage/src/messages.rs (1)

413-414: 💤 Low value

Consider importing HashSet at the top for consistency.

HashMap is imported at the file top (line 3) while HashSet is used with a full path here. Minor style inconsistency.

Suggested fix

At the top of the file:

-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};

Then here:

-let mut all_ids: std::collections::HashSet<EventId> = to_remove.iter().copied().collect();
+let mut all_ids: HashSet<EventId> = to_remove.iter().copied().collect();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/mdk-memory-storage/src/messages.rs` around lines 413 - 414, Import
HashSet alongside HashMap at the top of the file and replace the fully-qualified
type usage in the all_ids declaration; specifically, add HashSet to the existing
imports and change the line creating all_ids (currently using
std::collections::HashSet<EventId>) to use HashSet<EventId>, keeping the
variables to_remove, orphaned and type EventId unchanged.

crates/mdk-sqlite-storage/src/messages.rs (2)

390-399: ⚡ Quick win

Add an index for processed_messages.mls_group_id.

This new per-group cleanup path will full-scan processed_messages with the current schema shape. The visible index coverage only includes message_event_id, state, and processed_at, so disappearing-message cleanup cost grows with the entire dedup table instead of the target group.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/mdk-sqlite-storage/src/messages.rs` around lines 390 - 399, The
delete_processed_messages_for_group cleanup will full-scan processed_messages
because there is no index on processed_messages.mls_group_id; add a database
index for that column (e.g., CREATE INDEX IF NOT EXISTS on mls_group_id) as part
of your schema/migration so the delete in delete_processed_messages_for_group
can be satisfied by the index and avoid scanning the entire dedup table; ensure
the migration/initialization path that creates the processed_messages table also
creates the idx_processed_messages_mls_group_id index (or add a new migration
step) so existing deployments get the index applied.

1246-1253: ⚡ Quick win

Cover the file-backed secure_delete path too.

This assertion only exercises new_in_memory(). PRAGMA secure_delete is also set in open_connection(), and that persistent path is the security-sensitive one for on-disk recovery.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/mdk-sqlite-storage/src/messages.rs` around lines 1246 - 1253, Test
only covers the in-memory path; add a second test that constructs a file-backed
MdkSqliteStorage so open_connection() is exercised and then query "PRAGMA
secure_delete" via with_connection(...) and assert it equals 1; specifically,
keep the existing secure_delete_pragma_is_enabled test using
MdkSqliteStorage::new_in_memory() and add a new test that creates a
persistent/file-backed storage (so open_connection() runs), runs
conn.query_row("PRAGMA secure_delete", [], |row| row.get(0)) inside
with_connection, and asserts the returned i64 == 1.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/mdk-core/CHANGELOG.md`:
- Line 94: Update the changelog bullet for the Unreleased / Added entry that
mentions the public methods delete_message, delete_messages_before_timestamp,
and delete_processed_messages_for_group by appending the required PR reference
in the mandated format:
"([`#123`](https://github.com/marmot-protocol/mdk/pull/123))" (replace 123 with
the actual PR number that introduced these methods) so the line ends with the
trailing PR link.

In `@crates/mdk-memory-storage/CHANGELOG.md`:
- Line 59: Move the changelog line "Implemented `delete_message`,
`delete_messages_before_timestamp`, and `delete_processed_messages_for_group`
for per-message and bulk expiry-based deletion." out of the released `##
[0.8.0]` section and place it under `## Unreleased` within the `### Added`
subsection; then append the PR reference in the required format, e.g.
`([`#123`](https://github.com/marmot-protocol/mdk/pull/123))`, to the end of that
entry so it follows the project's changelog guidelines.

In `@crates/mdk-sqlite-storage/CHANGELOG.md`:
- Around line 60-61: Move the two bullets currently listed under the released
"0.8.0" section into the "## Unreleased" section of CHANGELOG.md and append the
PR reference suffix to each entry as
"([`#315`](https://github.com/your-repo/pull/315))"; specifically update the
bullets "Implemented `delete_message`, `delete_messages_before_timestamp`, and
`delete_processed_messages_for_group`..." and "Enabled `PRAGMA secure_delete =
ON`..." so they live under "## Unreleased" and end with the required PR link.

In `@crates/mdk-storage-traits/CHANGELOG.md`:
- Line 53: The two new changelog bullets that mention
MessageStorage::delete_message,
MessageStorage::delete_messages_before_timestamp, and
MessageStorage::delete_processed_messages_for_group are incorrectly placed under
the historical "0.8.0" section and lack the PR link; move these entries into the
"## Unreleased" section and append the PR reference
"([`#315`](https://github.com/marmot-protocol/mdk/pull/315))" to each bullet so
they follow the project's changelog convention.

In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 1291-1292: The conversion of the deletion result uses a lossy cast
(`as u32`) which can truncate large `usize` counts; replace the cast with a
checked conversion (e.g. `u32::try_from(count)` or equivalent) and map the Err
branch to an explicit error return so overflow is reported instead of silently
truncated—apply this change to the `let count =
mdk.delete_messages_before_timestamp(&group_id, before)?; Ok(count as u32)` site
and the analogous `count` -> `u32` conversion at the other occurrence around
lines 1304–1305, returning a clear error (with context like "deletion count
overflow") when the conversion fails.

---

Nitpick comments:
In `@crates/mdk-memory-storage/src/messages.rs`:
- Around line 413-414: Import HashSet alongside HashMap at the top of the file
and replace the fully-qualified type usage in the all_ids declaration;
specifically, add HashSet to the existing imports and change the line creating
all_ids (currently using std::collections::HashSet<EventId>) to use
HashSet<EventId>, keeping the variables to_remove, orphaned and type EventId
unchanged.

In `@crates/mdk-sqlite-storage/src/messages.rs`:
- Around line 390-399: The delete_processed_messages_for_group cleanup will
full-scan processed_messages because there is no index on
processed_messages.mls_group_id; add a database index for that column (e.g.,
CREATE INDEX IF NOT EXISTS on mls_group_id) as part of your schema/migration so
the delete in delete_processed_messages_for_group can be satisfied by the index
and avoid scanning the entire dedup table; ensure the migration/initialization
path that creates the processed_messages table also creates the
idx_processed_messages_mls_group_id index (or add a new migration step) so
existing deployments get the index applied.
- Around line 1246-1253: Test only covers the in-memory path; add a second test
that constructs a file-backed MdkSqliteStorage so open_connection() is exercised
and then query "PRAGMA secure_delete" via with_connection(...) and assert it
equals 1; specifically, keep the existing secure_delete_pragma_is_enabled test
using MdkSqliteStorage::new_in_memory() and add a new test that creates a
persistent/file-backed storage (so open_connection() runs), runs
conn.query_row("PRAGMA secure_delete", [], |row| row.get(0)) inside
with_connection, and asserts the returned i64 == 1.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cada1cf7-0394-4f92-9bd1-e9a4f492c7c7

📥 Commits

Reviewing files that changed from the base of the PR and between 25005ea and 2185662.

📒 Files selected for processing (10)

crates/mdk-core/CHANGELOG.md
crates/mdk-core/src/groups.rs
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-memory-storage/src/messages.rs
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-sqlite-storage/src/lib.rs
crates/mdk-sqlite-storage/src/messages.rs
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-storage-traits/src/messages/mod.rs
crates/mdk-uniffi/src/lib.rs

dannym-arx requested review from erskingardner, jgmontoya and mubarakcoded as code owners May 22, 2026 11:47

coderabbitai Bot requested changes May 22, 2026

View reviewed changes

Comment thread crates/mdk-core/CHANGELOG.md Outdated

Comment thread crates/mdk-memory-storage/CHANGELOG.md Outdated

Comment thread crates/mdk-sqlite-storage/CHANGELOG.md Outdated

Comment thread crates/mdk-storage-traits/CHANGELOG.md Outdated

Comment thread crates/mdk-uniffi/src/lib.rs

coderabbitai Bot approved these changes May 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deletion APIs + secure_delete for disappearing messages (3/3)#315

feat: deletion APIs + secure_delete for disappearing messages (3/3)#315
dannym-arx wants to merge 2 commits into
masterfrom
disappearing-messages-part-3

dannym-arx commented May 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 22, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

stage-review Bot commented May 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 22, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

dannym-arx commented May 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in it

Part of the split

Testing

What this doesn't do

Uh oh!

coderabbitai Bot commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

stage-review Bot commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Coverage: 94.51% → 94.44% (-0.07%)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dannym-arx commented May 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 22, 2026 •

edited

Loading

stage-review Bot commented May 22, 2026 •

edited

Loading

github-actions Bot commented May 22, 2026 •

edited

Loading